Benchmarks: Vortex queries

Verdict: No clear signal (low confidence)
Attributed Vortex impact: +3.1%
Engines: DataFusion No clear signal (+5.1%, low confidence) · DuckDB No clear signal (+1.1%, low confidence)
Vortex (geomean): 1.017x ➖
Parquet (geomean): 1.002x ➖
Shifts: Parquet (control) +0.2% · Median polish +0.7%

No file size changes detected.

Merging this PR will improve performance by 51.12%

⚡ 6 improved benchmarks
✅ 1691 untouched benchmarks
🆕 5 new benchmarks
⏩ 10 skipped benchmarks¹

Performance Changes

_{Comparing ad/cuda-fsst-varbin-export (2cc117e) with develop (54b045b)}

@claude review

Claude finished @0ax1's task in 3m 38s —— View job

Claude PR Review

Reviewed the full diff against origin/develop (PR base 159207e). This is a well-structured change with strong test coverage — the FSST→Arrow varbin path, the in-kernel view construction, and the BitBuffer::sliced Miri fix all hang together coherently. Findings below, ordered by importance.

🔴 Correctness / behavior

1. PR description is stale — no per-array fallback or fsst_varbin_offsets_fit predicate exists. The description says "FSST arrays whose decoded size exceeds i32::MAX bytes fall back to the view layout per-array. Schema derivation and data export share one predicate (fsst_varbin_offsets_fit)." Neither is in the code:

   • grep fsst_varbin_offsets_fit → no matches.
   • The actual behavior (confirmed by test_oversized_fsst_varbin_export_errors) is that oversized exports hard-error with "FSST decoded size exceeds Arrow i32 offset range" rather than falling back. Commit 3db845d ("fall back to view layout…") appears to have been superseded. Please update the PR description so reviewers/consumers aren't misled about the fallback semantics.

2. New default (Utf8) makes previously-exportable large string columns fail. With VarBinExportLayout::VarBin now the default, any Utf8 array whose decoded heap exceeds i32::MAX bytes — which previously exported fine as Utf8View — now returns an error instead of a valid array. This is inherent to choosing i32-offset Utf8, but it's a sharper edge than "the default layout changed": it's a silent-until-runtime failure for >2 GB string batches. Two things worth confirming:

   • Is erroring (vs. the originally-planned per-array view fallback) the intended final behavior? If so, the "Behavior changes" section should call this out explicitly so downstream consumers know to opt into with_varbin_export_layout(VarBinView) for large columns.
   • arrow_device_export_field unconditionally returns DataType::Utf8 for the schema even when the data export will error. Schema-succeeds / data-errors is a defensible "stable schema" choice (and is what test_oversized_fsst_varbin_export_errors asserts), but it directly contradicts the description's "always resolves to the same layout" claim.

🟡 Kernel / soundness

3. fsst_varbin values buffer relies on uncompressed_lengths being exact. output_offsets is the host prefix-sum of uncompressed_lengths (nulls contribute 0), and total_size is the last offset. The kernel writes each non-null string into [offset[sid], offset[sid+1]). If a decoded string were ever shorter than its claimed length, the gap bytes stay uninitialized device memory yet still fall inside total_size and get exported. This matches the pre-existing view-kernel contract, so it's not a regression — just flagging the invariant since the offset-based buffer has no per-view length to mask it.

4. In-kernel view construction (fsst_write_view) looks correct. Each thread reads back only the bytes it just wrote (no cross-thread reads, write-before-read ordered within the thread). The outlined branch's (uint32_t)start truncation is safe because views are only built when total_size <= MAX_BUFFER_LEN on the Rust side, keeping start within u32. The int32_t → uint64_t read of output_offsets in fsst_decode_string is safe given offsets are validated non-negative on the host. Good.

🟢 BitBuffer::sliced Miri fix

5. The rewrite to iter_padded().take(n_words) → BufferMut::<u64> → truncate is the right approach, and bundling it here makes sense since decode_fsst_varbin_typed now feeds validity…execute_mask(...).into_bit_buffer().sliced() straight to device FFI, which requires bit-0 alignment. Test coverage (aligned / unaligned / empty / crosses_word_boundary) is exactly the right matrix. Nice.

⚪ Minor / nits

• decode_fsst_varbin computes dtype, validity, and len, then decode_fsst_varbin_typed re-derives all three from fsst again. Cheap (clones/getters), but the duplication could be threaded through instead.
• Offset prefix-sum is done on the host (O(n) per export). cub::exclusive_sum_i32 is already imported in canonical.rs — a device sum would keep the whole path GPU-resident, though it's not required and matches the existing view path.
• The assert_eq!(output_base_ptr % 16, 0, …) alignment check panics rather than returning VortexResult. Consistent with the existing decode_fsst kernel, so fine, but it is a panic in library code.

📊 CI signals

• codspeed rank_single[(1024, 0.9)] −10.96%: vortex-mask rank does not call BitBuffer::sliced (it uses iter_padded directly in intersect_by_rank.rs), and this PR touches no mask code. Combined with codspeed's own "Unknown Walltime execution environment / Hosted Runners lead to inconsistent data" warning, this reads as measurement noise rather than a real regression. Worth a glance but likely ignorable.
• "SQL BENCHMARK FAILED": please confirm this is infra/unrelated and not triggered by the default Utf8View→Utf8 export change (a consumer expecting the view layout could break).

Overall: solid, thoroughly-tested change. The main asks are (1) fix the stale PR description and (2) confirm the oversized-export error (vs. fallback) is the intended contract and document the large-column implication.
· ad/cuda-fsst-varbin-export

I think the PR description is now stale, still refers to the fallback for >2G arrays

Polar Signals Profiling Results

Latest Run

Powered by Polar Signals Cloud